-
Notifications
You must be signed in to change notification settings - Fork 174
docker: use node lts and run the app as flood user #837
Conversation
Thanks @mycodeself! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor indentation change request, sorry
.prettierrc \ | ||
ABOUT.md \ | ||
$WORKDIR | ||
package-lock.json \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we restore the previous, unless there's a good reason to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous indentation has 5 spaces. The rest of file has 4 spaces, i think it should use the same. What do you think? If you want to mantain 5 spaces in these lines, i can revert it.
RUN addgroup -S flood | ||
RUN adduser -S flood -G flood | ||
RUN mkdir /data | ||
RUN chown flood:flood /data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned if adding a user now will break existing setups, looking at existing docker volume for /data
currently there are files owned by the root
user which when the app runs under the flood
user would in theory not have access the database file. Rebuilding the container will not change the files/owners of existing files on docker volumes.
I will have a quick test on my local setup to see what effects there are on my setup when a build new image with these changes, may require users to manually run chown
after re-building the image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you been able to test how it affects current configurations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MacTwister maybe the file are own by root because the docker image was built and the default user was root but if you build it and add another user the file will be own by another user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so now if you build the docker image on your side using this Dockerfile (with chown changes) then the app will be owned and run as the flood
user. 👍
For new setups using docker:
New container will run as flood
user and create the database under /data
owned by the flood
user. All good here.
For existing setups using docker:
The scenario I am describing is for people already running with Docker and have setup the /data
folder as a "volume" to persist the DB when restarting/upgrading the docker image. Below is simplified version to how I have my setup:
# ... rest of you docker compose file
floodui:
build: .
ports:
- 3000:3000
volumes:
- flood_data:/data
So when re-creating the container with new image, the app will crash with the following error:
[Error: EACCES: permission denied, open '/data/server/db/users.db'] {
This is because my volume /data
is persisted over from the previous container, where it was first created (when container was root only) so my version of /data
is owned by root:root
.
julian$ docker-compose run --rm floodui ls -l /data
total 4
drwxr-xr-x 3 root root 4096 Oct 3 08:41 server
What I am saying is that, if we go through with this change, we'd need to some how inform users what they would need to do to continue working after this upgrade. i.e. update the docs for docker setup to inform the to update the /data ownership manually.
What I needed to do using my docker compose setup to continue working:
julian$ docker-compose run --user=root --rm floodui chown -R flood:flood /data
Co-Authored-By: Julian G <[email protected]>
There is not really a need for this because the container can be started with a different user and group and everything works just fine. I would never have used flood if it would require root privileges. Simply use the |
Changes to the dockerfile.
Description
The dockerfile has been updated with some improvements:
docker build --build-arg FLOOD_BASE_URI=/flood/ .
.flood
. The /data directory and the $WORKDIR are owned by this user.Related Issue
#739
#621
Motivation and Context
How Has This Been Tested?
docker build --build-arg FLOOD_BASE_URI=/flood/ -t flood . && docker run -p 3000:3000 --name flood flood
http://127.0.0.1:3000/flood/
Screenshots (if appropriate):
Types of changes
Checklist: